-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Develop #4815
base: master
Are you sure you want to change the base?
Develop #4815
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there! 👋 It looks like there are a few important areas that need some attention before we can approve your Pull Request. Specifically, there's a misnamed BEM modifier that doesn't match its functionality, some missing file extensions in import statements, and a few other issues like typos and missing image references. These are crucial for maintaining the integrity and readability of the code. Don't worry, with some quick fixes, you'll be on the right track! Remember, every coder starts somewhere, and every mistake is an opportunity to learn and grow. Keep up the good work! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<link | ||
rel="preconnect" | ||
href="https://fonts.googleapis.com" | ||
/> | ||
<link | ||
rel="preconnect" | ||
href="https://fonts.gstatic.com" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Code Style Rules ❗️, you should combine the preconnect
link tags into one. You can specify multiple href
attributes separated by a comma.
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__sudtitle">Product code: 195434</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo in the class name card__sudtitle
. It should be card__subtitle
to follow proper English spelling and to be consistent with BEM naming conventions.
class="images" | ||
></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty links are not allowed. If this is supposed to be an image container, you should include an img
tag inside the a
tag with the appropriate src
and alt
attributes.
<link | ||
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;0,800;0,900;1,100;1,200;1,300;1,400;1,500;1,600;1,700;1,800;1,900&display=swap" | ||
rel="stylesheet" | ||
/> | ||
<link | ||
rel="stylesheet" | ||
href="./styles/index.scss" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute should point to a .css
file, not a .scss
file. Ensure that the SCSS files are being compiled to CSS and that the compiled CSS file is correctly linked in the href
attribute.
&__sudtitle { | ||
margin: 4px 0 0; | ||
padding-top: 0; | ||
font-weight: 400; | ||
line-height: 14px; | ||
color: #616070; | ||
font-size: 10px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears there is a typo in the BEM element name &__sudtitle
. It should be &__subtitle
to follow the proper naming convention and maintain consistency.
&__buy-button { | ||
margin: 16px 0 0; | ||
background-color: #00acdc; | ||
position: relative; | ||
width: 166px; | ||
height: 40px; | ||
border-radius: 5px; | ||
color: #fff; | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
text-align: center; | ||
font-weight: 700; | ||
font-size: 14px; | ||
line-height: 16px; | ||
text-decoration: none; | ||
box-sizing: border-box; | ||
&:hover { | ||
background: #fff; | ||
color: #00acdc; | ||
border: 1px solid #00acdc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the hover state for the &__buy-button
is consistent with the design requirements. If the design specifies different colors or styles, they should be implemented accordingly. Additionally, when changing background-color
on hover, it's better to transition the color change for a smoother visual effect. Consider adding transition: background-color 0.3s;
to the &__buy-button
block.
.images { | ||
background-image: url(../images/imac.jpeg); | ||
background-repeat: no-repeat; | ||
background-size: cover; | ||
display: block; | ||
width: 160px; | ||
height: 134px; | ||
margin: 0 3px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class name .images
suggests that it can be applied to multiple elements, but the styles within are very specific to a single image. Consider renaming the class to more accurately reflect its purpose, such as .product-image
or .card-image
, to improve readability and maintainability of the code.
@@ -0,0 +1,9 @@ | |||
.images { | |||
background-image: url(../images/imac.jpeg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Code Style Rules ❗️, for SCSS files, it is recommended to use variables for colors, fonts, and other static values. Consider storing image paths as variables at the top of your SCSS files for easier maintenance and reusability.
body { | ||
@import '../styles/card'; | ||
@import '../styles/image'; | ||
@import '../styles/starts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Code Style Rules, when importing several files, each import statement should be on a new line and should not have a file extension. The import statements here are correctly placed on separate lines, but the file extensions are missing. Please add '.scss' to the end of each import path.
&--0 { | ||
.stars__star:nth-child(-n + 4) { | ||
background-image: url(../images/star-active.svg); /* Active star image */ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BEM modifier stars--0
seems to be misnamed. According to the checklist, modifiers should represent different states or versions of a block or an element. However, stars--0
implies it is for zero stars, which doesn't make sense as this code is setting the star-active
image for the first four stars. Ensure that the modifier name and the associated style rules accurately represent the intended design and functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job!
background-repeat: no-repeat; | ||
background-size: cover; | ||
display: block; | ||
width: 160px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this style looks redundant
DEMO LINK
Я маю проблеми з деплоєм і ментор сказав пушити так
https://ibb.co/3vXxmX4(посилання на фото тестів)